Conversation
…ostlygeek#421) Replace container/ring.Ring with a custom circularBuffer that uses a single contiguous []byte slice. This fixes the original implementation which created 10,240 ring elements instead of 10KB of storage. GetHistory is now 139x faster (145μs → 1μs) and uses 117x less memory (1.2MB → 10KB). Allocations reduced from 2 to 1 per write operation. Create a LogMonitor per proxy.Process, replacing the usage of a shared one. The buffer in LogMonitor is lazy allocated on the first call to Write and freed when the Process is stopped. This reduces unnecessary memory usage when a model is not active. The /logs/stream/{model_id} endpoint was added to stream logs from a specific process.
A {model_id} containing a forward slash trips up gin's path param
parsing. This updates /logs/stream to work like /upstream where the
model_id is built up in parts and searched for in the configuration.
Updates mostlygeek#421
The new logToStdout option controls what is logged to stdout. The default has been changed to just the proxy logs, which contain swap and http request logs. There are four supported settings: none, proxy, upstream, both. The "both" setting is the legacy setting where everything was spewed to stdout.
WalkthroughThis PR introduces configurable stdout logging behavior, per-model log stream access, and architectural improvements to log monitoring. Configuration additions enable granular control over logging destinations (proxy, upstream, both, or none), while ProxyManager refactoring implements dynamic model-based log routing and a planned circular buffer architecture for efficient log buffering. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
proxy/process.go (1)
147-150: Duplicate accessor methods for the same field.Both
LogMonitor()(line 148) andLogger()(line 895) returnp.processLogger. This creates API confusion and maintenance overhead.Proposed fix: Remove one of the duplicate methods
Either remove
LogMonitor()orLogger()and update all call sites to use the remaining method. Based on usage inproxymanager_loghandlers.go,Logger()appears to be the intended API:-// LogMonitor returns the log monitor associated with the process. -func (p *Process) LogMonitor() *LogMonitor { - return p.processLogger -} -Also applies to: 894-897
🧹 Nitpick comments (1)
proxy/logMonitor_test.go (1)
257-267: Subscriber cleanup functions are not called.
OnLogDatareturns a cleanup function that should be called to unsubscribe. While this may not significantly impact benchmark accuracy, it's a resource leak pattern.Proposed fix
b.Run("WithSubscribers", func(b *testing.B) { lm := NewLogMonitorWriter(io.Discard) // Add some subscribers + cleanups := make([]func(), 5) for i := 0; i < 5; i++ { - lm.OnLogData(func(data []byte) {}) + cleanups[i] = lm.OnLogData(func(data []byte) {}) } + b.Cleanup(func() { + for _, cleanup := range cleanups { + cleanup() + } + }) b.ResetTimer() for i := 0; i < b.N; i++ { lm.Write(mediumMsg) } })
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
LICENSE.mdREADME.mdai-plans/2025-12-14-efficient-ring-buffer.mdconfig.example.yamldocs/configuration.mdproxy/config/config.goproxy/config/config_posix_test.goproxy/config/config_windows_test.goproxy/logMonitor.goproxy/logMonitor_test.goproxy/process.goproxy/processgroup.goproxy/proxymanager.goproxy/proxymanager_loghandlers.goproxy/proxymanager_test.go
🧰 Additional context used
📓 Path-based instructions (2)
proxy/**/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
Run
make test-devwhen making iterative changes to code under theproxy/directory - includesgo testandstaticcheck
Files:
proxy/proxymanager_test.goproxy/processgroup.goproxy/logMonitor_test.goproxy/config/config_windows_test.goproxy/proxymanager_loghandlers.goproxy/config/config_posix_test.goproxy/config/config.goproxy/proxymanager.goproxy/process.goproxy/logMonitor.go
ai-plans/**
📄 CodeRabbit inference engine (CLAUDE.md)
ai-plans/**: Work plans located in ai-plans/ should identify any inconsistencies when expanded
Work plans should be expanded with detailed specifications including Title, Overview, Design Requirements, Testing Plan, and Checklist sections
Files:
ai-plans/2025-12-14-efficient-ring-buffer.md
🧠 Learnings (1)
📚 Learning: 2025-12-11T12:43:01.580Z
Learnt from: CR
Repo: napmany/llmsnap PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-11T12:43:01.580Z
Learning: Applies to proxy/**/*.go : Run `make test-dev` when making iterative changes to code under the `proxy/` directory - includes `go test` and `staticcheck`
Applied to files:
proxy/proxymanager_test.goproxy/config/config_windows_test.goproxy/config/config_posix_test.go
🧬 Code graph analysis (5)
proxy/processgroup.go (2)
proxy/logMonitor.go (1)
NewLogMonitorWriter(125-134)proxy/process.go (2)
NewProcess(101-145)Process(54-99)
proxy/logMonitor_test.go (1)
proxy/logMonitor.go (1)
NewLogMonitorWriter(125-134)
proxy/config/config_windows_test.go (1)
proxy/config/config.go (1)
LogToStdoutProxy(19-19)
proxy/config/config_posix_test.go (1)
proxy/config/config.go (1)
LogToStdoutProxy(19-19)
proxy/logMonitor.go (1)
event/event.go (1)
Dispatcher(30-35)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: run-tests
- GitHub Check: run-tests
🔇 Additional comments (30)
README.md (2)
195-195: Code fence language consistency improved.Changed from
shelltoshfor conciseness. This is a minor stylistic improvement that maintains clarity.
200-216: Excellent documentation of new logging endpoints.The new examples clearly document the enhanced logging capabilities:
curl -Ns http://host/logs/stream/proxy— proxy status logscurl -Ns http://host/logs/stream/upstream— upstream process logscurl -Ns http://host/logs/stream/{model_id}— per-model logsThe
?no-historyparameter is well-documented. Examples use consistent formatting with shell prompts for easy copy-paste, and comments explain each endpoint's purpose.LICENSE.md (1)
4-4: LGTM!The copyright addition for 2025 is appropriate and follows standard MIT license practices.
proxy/config/config.go (4)
18-23: LGTM!The exported constants for logging targets are well-defined with clear, descriptive names. The values ("proxy", "upstream", "both", "none") are intuitive and align with the documentation.
125-125: LGTM!The LogToStdout field is properly added to the Config struct with appropriate YAML tag for configuration file binding.
191-191: LGTM!Defaulting to LogToStdoutProxy is a sensible choice, maintaining backward compatibility by showing proxy logs (the existing behavior).
218-222: LGTM!The validation logic properly enforces the allowed values and provides a clear error message listing all valid options.
docs/configuration.md (1)
93-95: LGTM!The documentation additions are comprehensive and well-structured:
- Clear descriptions of each new configuration option
- Proper documentation of default values and valid values
- Good usage guidance for logToStdout options
- Per-model override capability properly documented for sendLoadingState
Also applies to: 121-137, 163-175, 325-327
proxy/config/config_windows_test.go (1)
161-161: LGTM!The test is correctly updated to expect the new LogToStdout field with its default value.
proxy/logMonitor.go (3)
14-91: LGTM! Well-implemented circular buffer.The custom circular buffer implementation is correct and efficient:
- Proper handling of wrap-around when data crosses the buffer boundary (lines 48-58)
- Correct calculation of the oldest data position using
(head - size + cap) % cap(line 78)- Handles edge case where input exceeds capacity by keeping only the last
capbytes (lines 40-45)- GetHistory correctly reconstructs data in chronological order for both contiguous and wrapped cases (lines 80-88)
The replacement of ring.Ring with this fixed-capacity buffer provides predictable O(1) writes and better control over memory usage.
101-101: LGTM!The LogBufferSize constant (100 KB) provides a reasonable default for log history buffering without excessive memory consumption.
128-128: LGTM! Efficient lazy initialization pattern.The lazy initialization approach (buffer is nil until first Write) combined with the Clear() method enables efficient memory management:
- Memory is only allocated when actually needed
- Clear() allows releasing buffer memory when not in use
- Thread-safe implementation with proper lock usage
Also applies to: 147-150, 169-175
proxy/config/config_posix_test.go (1)
169-169: LGTM!The test is correctly updated to expect the new LogToStdout field with its default value, maintaining consistency with the Windows test.
config.example.yaml (1)
49-57: LGTM!The logToStdout configuration is well-documented with clear descriptions of each valid value and its behavior. The inline comments provide excellent user guidance.
proxy/proxymanager_test.go (1)
1081-1082: LGTM!The test addition validates per-model log stream functionality and ensures the endpoint correctly handles model names containing slashes ("author/model"), which is important for namespaced model identifiers.
Also applies to: 1095-1095
proxy/process.go (1)
613-618: Good addition for memory management.Clearing the process logger buffer in the defer block ensures memory is released when a process stops, which aligns with the lazy initialization pattern in LogMonitor.
proxy/processgroup.go (2)
49-51: LGTM - Per-process logger wiring.Creating a dedicated
LogMonitorfor each member process enables per-model log streams. The logger correctly wrapsupstreamLoggerto maintain the logging hierarchy.
93-98: LGTM - GetMember accessor.The method correctly checks membership before returning the process. This works safely because
pg.processesis populated only in the constructor and is immutable thereafter.proxy/proxymanager_loghandlers.go (2)
34-34: LGTM - Path parameter normalization.Trimming the leading slash from
logMonitorIDensures consistent handling regardless of how the path is captured.
86-106: Clean refactor to switch-based logger resolution.The switch statement improves readability over conditional chains. The model-based lookup via
findModelInPathenables per-model log streams.One minor observation: the error message at line 105 mentions
'proxy', 'upstream' or a model's IDbut doesn't indicate which model IDs are valid. Consider whether listing available models would be helpful for debugging.proxy/logMonitor_test.go (4)
117-144: Comprehensive circular buffer wrap-around tests.Good coverage of wrap-around behavior including overwriting oldest data and handling writes larger than buffer capacity.
146-166: Good boundary condition coverage.Tests correctly verify empty buffer, exact capacity, and boundary writes.
168-225: LGTM - LogMonitor lifecycle tests.Tests properly verify lazy initialization, buffer clearing, and reuse after clear. These validate the memory management improvements.
282-316: Good documentation of benchmark results.The commented benchmark comparison clearly shows the performance improvements from the circular buffer implementation (~139x faster GetHistory, reduced allocations).
ai-plans/2025-12-14-efficient-ring-buffer.md (1)
1-85: Well-structured work plan.The plan includes all required sections (Title, Overview, Design Requirements, Testing Plan, Checklist) and clearly documents the rationale for replacing
ring.Ring. The design requirements and testing plan align with the implementation observed in the test file.Based on coding guidelines, this work plan follows the expected format for
ai-plans/**files.proxy/proxymanager.go (5)
55-79: Clean LogToStdout configuration handling.The switch statement handles all logging destination combinations. The default case (lines 72-78) provides sensible backward compatibility for configs that don't explicitly set
LogToStdout.
489-514: LGTM - Model path resolution.The iterative approach correctly handles model names containing slashes (e.g., "author/model"). Empty path segments are properly skipped, and the remaining path is correctly calculated.
516-542: Correct redirect handling for model paths.Using 308 (Permanent Redirect) for non-GET/HEAD requests correctly preserves the HTTP method. Query parameter preservation ensures upstream API compatibility. The logic correctly distinguishes between exact model matches needing a trailing slash redirect vs. paths with additional segments.
287-289: LGTM - Log stream route addition.The wildcard route
/logs/stream/*logMonitorIDcorrectly enables per-model log streaming endpoints while maintaining backward compatibility with the base/logs/streamroute.
157-160: LGTM - Process group creation with new config.Process groups are correctly created using
proxyConfigand the updated logger references.
2 participants
Summary by CodeRabbit
Release Notes
New Features
Documentation
Tests
✏️ Tip: You can customize this high-level summary in your review settings.